Skip to content

[Audit log forwarding] - update audit-tls secret to IM deployments for audit forwarding support #1061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

[Audit log forwarding] - update audit-tls secret to IM deployments for audit forwarding support #1061

wants to merge 21 commits into from

Conversation

rashmi43
Copy link
Member

@rashmi43 rashmi43 commented Jul 2, 2025 •
edited
Loading

@rashmi43 rashmi43 requested a review from rwhundley August 4, 2025 16:20
@rashmi43 rashmi43 changed the title [Audit log forwarding] - watch audit-tls secret to update deployments for audit [Audit log forwarding] - update audit-tls secret to IM deployments for audit forwarding support Aug 4, 2025
rwhundley
rwhundley requested changes Aug 4, 2025
View reviewed changes
rashmi43 added 3 commits August 4, 2025 22:02
@rashmi43
update variable name
c5b6d91 
Signed-off-by: rashmi_kh <[email protected]>
@rashmi43
remove watch
26a0204 
Signed-off-by: rashmi_kh <[email protected]>
@rashmi43
remove watch
1d61bf7 
Signed-off-by: rashmi_kh <[email protected]>
rwhundley
rwhundley requested changes Aug 4, 2025
View reviewed changes
bundle/manifests/operator.ibm.com_authentications.yaml Outdated
@@ -228,6 +228,10 @@ spec:
type: string
providerIssuerURL:
type: string
auditURL:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes should also be happening in the config directory, and changes here should be generated with make bundle

rashmi43 and others added 4 commits August 4, 2025 22:31
rwhundley
rwhundley requested changes Aug 4, 2025
View reviewed changes
internal/controller/operator/configmap.go
@@ -345,6 +345,10 @@ func updatePlatformAuthIDP(_ common.SecondaryReconciler, _ context.Context, obse
"LDAP_CTX_POOL_PREFERREDSIZE"),
updatesValuesWhen(not(observedKeySet[*corev1.ConfigMap]("MASTER_PATH")),
"MASTER_PATH"),
updatesValuesWhen(not(observedKeySet[*corev1.ConfigMap]("AUDIT_URL")),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In conjunction with this comment, it's my feeling that these should just be added to the updatesAlways call, and, if we're trying to account for an existing setting on the ConfigMap from an upgrade, then we should account for that in the bootstrap controller.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're still missing changes to the bootstrap controller, and this update function call is no longer needed once that is done.

rashmi43 and others added 3 commits August 5, 2025 14:24
rwhundley
rwhundley requested changes Aug 8, 2025
View reviewed changes
@rashmi43
review comments
f16e9b9 
Signed-off-by: rashmi_kh <[email protected]>
@rashmi43
add print
b29c9f2 
Signed-off-by: rashmi_kh <[email protected]>
@rashmi43
remove print
624a0e6 
Signed-off-by: rashmi_kh <[email protected]>
@rashmi43 rashmi43 requested a review from rwhundley August 13, 2025 13:30
@rashmi43
Copy link
Member Author

works as expected:

{"level":"info","ts":"2025-08-13T14:02:12Z","msg":"Fetched audit URL and audit Secret from Authentication CR","AUDIT_SECRET":"audit-tls","AUDIT_URL":"https://myauditservice:9080"}
{"level":"info","ts":"2025-08-13T14:02:12Z","msg":"Secret for audit configuration not found"}
{"level":"info","ts":"2025-08-13T14:02:12Z","logger":"controller_authentication","msg":"Does audit-tls secret exist?","Request.Namespace":"mcsp-on-4-14","Request.Name":"example-authentication","subreconciler":"handleDeployments","Deployment.Namespace":"mcsp-on-4-14","Secret exists":"audit-tls"}

@rashmi43
Copy link
Member Author 

{"level":"info","ts":"2025-08-13T13:35:51Z","msg":"Fetched audit URL and audit Secret from Authentication CR","AUDIT_SECRET":"nil","AUDIT_URL":"https://myauditservice:9080"}
{"level":"info","ts":"2025-08-13T13:35:51Z","msg":"Secret for audit configuration not found"}
{"level":"info","ts":"2025-08-13T13:35:51Z","logger":"controller_authentication","msg":"Does audit-tls secret exist?","Request.Namespace":"mcsp-on-4-14","Request.Name":"example-authentication","subreconciler":"handleDeployments","Deployment.Namespace":"mcsp-on-4-14","Secret exists":"nil"}

@rashmi43
Copy link
Member Author

when its nil:

{"level":"info","ts":"2025-08-13T13:35:51Z","msg":"Fetched audit URL and audit Secret from Authentication CR","AUDIT_SECRET":"nil","AUDIT_URL":"https://myauditservice:9080"}
{"level":"info","ts":"2025-08-13T13:35:51Z","msg":"Secret for audit configuration not found"}
{"level":"info","ts":"2025-08-13T13:35:51Z","logger":"controller_authentication","msg":"Does audit-tls secret exist?","Request.Namespace":"mcsp-on-4-14","Request.Name":"example-authentication","subreconciler":"handleDeployments","Deployment.Namespace":"mcsp-on-4-14","Secret exists":"nil"}

rwhundley
rwhundley requested changes Aug 13, 2025
View reviewed changes
Copy link
Collaborator

@rwhundley rwhundley left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make test fails when I pull this locally. Please address this.

Edit: Also be sure to add api/operator/v1alpha1/zz_generated.deepcopy.go to your changes as it is updated when code generation is performed. Run make generate and check git status to see what I'm talking about. Thanks.

@rashmi43
remove print
6e9b29d 
Signed-off-by: rashmi_kh <[email protected]>
@rashmi43
final changes
e80e1ec 
Signed-off-by: rashmi_kh <[email protected]>
@rashmi43
Copy link
Member Author

make test fails when I pull this locally. Please address this.

Edit: Also be sure to add api/operator/v1alpha1/zz_generated.deepcopy.go to your changes as it is updated when code generation is performed. Run make generate and check git status to see what I'm talking about. Thanks.

fixed

@rashmi43 rashmi43 requested a review from rwhundley August 14, 2025 07:16
Tirumalavasa
Tirumalavasa approved these changes Aug 14, 2025
View reviewed changes
Copy link
Member

@Tirumalavasa Tirumalavasa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am fine with the changes, once Rob's review comments are addressed, he can check and merge it

@ibm-ci-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rashmi43, Tirumalavasa

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Tirumalavasa,rashmi43]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rwhundley
rwhundley requested changes Aug 18, 2025
View reviewed changes
internal/controller/operator/configmap.go
@@ -345,6 +345,10 @@ func updatePlatformAuthIDP(_ common.SecondaryReconciler, _ context.Context, obse
"LDAP_CTX_POOL_PREFERREDSIZE"),
updatesValuesWhen(not(observedKeySet[*corev1.ConfigMap]("MASTER_PATH")),
"MASTER_PATH"),
updatesValuesWhen(not(observedKeySet[*corev1.ConfigMap]("AUDIT_URL")),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're still missing changes to the bootstrap controller, and this update function call is no longer needed once that is done.

internal/controller/operator/configmap.go
@@ -538,6 +548,12 @@ func (r *AuthenticationReconciler) generateAuthIdpConfigMap(clusterInfo *corev1.
},
}

if authCR.Spec.Config.AuditUrl != nil && authCR.Spec.Config.AuditSecret != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary - something is broken if you are not seeing updatesAlways set the correct values on the ConfigMap. I'd assume it's because the bootstrap controller changes I mentioned earlier have not been done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwhundley rwhundley rwhundley requested changes

@Tirumalavasa Tirumalavasa Tirumalavasa approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rashmi43 @ibm-ci-bot @Tirumalavasa @rwhundley